Union query result should work in JDBC format #2757
Conversation
Signed-off-by: Lantao Jin <[email protected]>
LantaoJin
requested review from
ps48,
kavithacm,
derek-ho,
joshuali925,
dai-chen,
YANG-DB,
rupal-bq,
mengweieric,
vamsimanohar,
Swiddis,
penghuo,
seankao-az,
MaxKsyunz,
Yury-Fridlyand,
anirudha,
forestmvey,
acarbonetto,
GumpacG and
ykmr1224
as code owners
| String query = | ||
| String.format( | ||
| Locale.ROOT, | ||
| "SELECT firstname, lastname FROM %s LIMIT 3 " |
There was a problem hiding this comment.
does UNION works for query with aggregation?
There was a problem hiding this comment.
Actually, UNION with Agg in legacy engine has bugs by my testing whatever format set. This fixing doesn't cover all existed bugs. You can try below queries.
POST _plugins/_sql?format=csv
{
"query": "select customer_gender,count(type) from opensearch_dashboards_sample_data_ecommerce group by customer_gender UNION select customer_gender,count(type) from opensearch_dashboards_sample_data_ecommerce group by customer_gender"
}
POST _plugins/_sql?format=csv
{
"query": "select count(type) from opensearch_dashboards_sample_data_ecommerce group by customer_gender UNION select count(type) from opensearch_dashboards_sample_data_ecommerce group by customer_gender"
}
May be we could open another ticket to fix the problems in Union with Agg? Or migrating to v2?
There was a problem hiding this comment.
And seems current implementation doesn't distinguish UNION and UNION ALL. A better way is to re-implement UNION in v2, PPL also requests multi-search implementation.
Again, this fixing is just for addressing the NPE problem of UNION query in JDBC format (such a common use case).
|
Can you add little more details on where exactly is the bug? |
| @@ -105,7 +105,7 @@ private List<String> getFormatsForColumn(String columnName) { | |||
| private Set<String> getDateColumns(List<Schema.Column> columns) { | |||
| return columns.stream() | |||
| .filter(column -> column.getType().equals(Schema.Type.DATE.nameLowerCase())) | |||
| .map(Schema.Column::getName) | |||
| .map(Schema.Column::getIdentifier) | |||
There was a problem hiding this comment.
Is this related to the current issue?
There was a problem hiding this comment.
Good catching! This change should be reverted. What change we need for alias case is just this diff: https://github.com/opensearch-project/sql/pull/2757/files#diff-943465139286a5010c19cab380f141e3c9bc26fb02652bedbc6a7bbec5c98cc3R46
| @@ -22,7 +23,7 @@ public class MultiQueryAction extends QueryAction { | |||
| private MultiQuerySelect multiQuerySelect; | |||
|
|
|||
| public MultiQueryAction(Client client, MultiQuerySelect multiSelect) { | |||
| super(client, null); | |||
| super(client, new Union(multiSelect.getFirstSelect(), multiSelect.getSecondSelect())); | |||
There was a problem hiding this comment.
Is this the root cause of the issue?
There was a problem hiding this comment.
Yes, plus missing call loadFromEsState for both side of Union.
Signed-off-by: Lantao Jin <[email protected]>
The NPE issue caused by calling |
| @@ -180,8 +185,12 @@ private void loadFromEsState(Query query) { | |||
| Map<String, FieldMappingMetadata> typeMappings = mappings.get(indexName); | |||
|
|
|||
| this.indexName = this.indexName == null ? indexName : (this.indexName + "|" + indexName); | |||
| this.columns.addAll( | |||
| renameColumnWithTableAlias(query, populateColumns(query, fieldNames, typeMappings))); | |||
| if (isPartOfUnion(query) && !this.columns.isEmpty()) { | |||
There was a problem hiding this comment.
What's the difference between isUnionQuery and isPartOfUnion? Do we need the new partOfUnion field?
There was a problem hiding this comment.
isUnionQuery checks if the query type is Union, just similar to isJoinQuery does, checking if the type is Join.
The children type of Join is TableOnJoinSelect, a special type rather than a normal Select. But The children type of Union is a normal Select, unless we set a new type for Union's children, we can't identify a Select is a normal one or a child of Union in loadFromEsState, that's the reason I add partOfUnion in Select. Or add a new class class UnionSubSelect extends Select. Which one would be better?
https://github.com/opensearch-project/sql/pull/2757/files#diff-0b55e630b732c43e3c8ea8038eb068f21a3c46bcd1744c98be4cc94bad799b84R50
|
This PR is stalled because it has been open for 30 days with no activity. |
Description
Unionquery only works withcsvformat, but without the '?format=csv' query parameter at the end of the POST call will throwNullPointerException. Reproduce:Currently, the
UNIONstatement is implemented in legacy engine(v1). It's a bug in legacy engine(v1).This PR fixes this issue in legacy engine.
Issues Resolved
Resolve #2540
Check List
[ ] New functionality has been documented.[ ] New functionality has javadoc added[ ] New functionality has user manual doc addedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.